Add functions copied from Backdrop core module_enable() and #445
Conversation
module_disable() functions, and utilize them when enabling and disabling modules to notify the user of dependent and/or required modules that are being disabled/enabled in addition to the named modules. Issue backdrop-contrib#164
|
Hi @elisseck - If you put I've had a quick look and initial feedback I would give is that messages saying 'will also be' should use the |
|
Cool, thank you! I'll wait for your detailed review and then I'm happy to change the |
There was a problem hiding this comment.
Hi @elisseck
Thank you for your efforts on this.
I've gone through this and the code looks mostly good (a minor typo)
I don't know if this is feasible but if I enable the submodule the messages aren't quite right. 'Entity Plus' is a dependency of 'Paragraphs'. 'Paragraphs' is the only dependency of 'Paragraphs Type Permissions'
lando bee en paragraphs_bundle_permissions
ℹ The 'Entity Plus' module will also be enabled, as it is required by the 'Paragraphs Type Permissions' module.
ℹ The 'Paragraphs' module will also be enabled, as it is required by the 'Paragraphs Type Permissions' module.
✔ The 'Entity Plus' module was enabled.
✔ The 'Paragraphs' module was enabled.
✔ The 'Paragraphs Type Permissions' module was enabled.
I don't think this is a deal breaker if not as this is still a big improvement. Maybe an alternative is to clarify the message to say:
The 'Entity Plus' module will also be enabled, as it is required by the 'Paragraphs Type Permissions' module or one of the modules it requires.
As previously mentioned, replacing bee_message() with bee_instant_message() where the message talks about what the function will do, will make more sense for the user.
Also, as it is envisaged that this will be a temporary solution until the functions are split out in core, I propose adding a new file in includes called dependencies.inc and include this in bee.php. I don't know how long the core change will take so this will keep it tidy and reduce the possibilities of merge conflicts later on.
Also, it would be helpful to add a line under 'Changed' (will need adding under Unreleased) in the CHANGELOG. Update the date for the Unreleased section to whichever date you submit the changes. I'll update if needed.
elisseck
force-pushed
the
164-increase-module-dependency-verbosity
branch
2 times, most recently
from
cd9a81f
to
90d51bd
Compare
- Pull temporary code into its own file includes/dependencies.inc - Make use of bee_instant_message() where appropriate - Add clarity to user message when a dependency of a dependency is disabled - Fix code comment typo abled() -> enabled() - Updated changelog Issue backdrop-contrib#164
elisseck
force-pushed
the
164-increase-module-dependency-verbosity
branch
from
90d51bd
to
79cf5cb
Compare
|
Thanks for your review @yorkshire-pudding! All makes sense to me! I had a little trouble getting my editor to play nice with phpcs but I believe I made the changes you requested, including that typo fix although i'm not sure the formal github change request resolved since it's now in a different file. I went with your suggestion for the note on "dependencies of dependency" module situations... the backdrop core function doesn't give us that information right now, so i'd rather not change it. We can PR the change to backdrop core moving the code as-is, and then maybe PR the change to get the dependency info we'd need to properly report this to users after that. |
Fixes #164
This PR works towards resolving issue #164, attempting to give better user feedback when enabling/disabling modules that have requirements/dependencies.
Currently, we are copying what Backdrop core does in the
module_enable()andmodule_disable()functions to notify the user of dependent and/or required modules that are being disabled/enabled in addition to the named modules when calling e.g.bee dis my_moduleorbee en my_module.Ideally, this is a stopgap method, and an issue will be filed against core to pop the functionality I've copied into
miscellaneous.incoutside ofmodule_enable()andmodule_disable()so it is available to contrib modules. At that point we can use that feature and stop doing the dependency calculation twice during the process.If there is agreement on this approach I will open the issue against Core and do the same for the install/uninstall callbacks here in
bee. Let me know what you think!Example output before, missing crucial information:
Example after: